-
Notifications
You must be signed in to change notification settings - Fork 47
Test lowest-direct dependencies #858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #858 +/- ##
===========================================
+ Coverage 89.41% 90.79% +1.37%
===========================================
Files 63 66 +3
Lines 4857 5160 +303
===========================================
+ Hits 4343 4685 +342
+ Misses 514 475 -39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @agriyakhetarpal, I'm hoping you may have some advice! |
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @NicolaCourtier, I actually asked about this in the Scientific Python Discord server previously, but I forgot to respond to you here. Apologies! The command you have mentioned is the closest that will work for us here. I think the issue is that you are using pip instead of uv pip?
Besides this, one option is to adopt SPEC 0: https://scientific-python.org/specs/spec-0000/. It features a companion GitHub Action that we can use. However, PyBaMM has not adopted SPEC 0 yet, which is why I think it may not be the best idea for PyBOP. I think it would still be worth experimenting, however. See pybamm-team/PyBaMM#5202 for further discussion.
Another option I am aware of is to use a file like this: https://github.com/scikit-learn/scikit-learn/blob/ce1b377f3bb2580e7cc1e35ba7eed2131e3b8e8e/sklearn/_min_dependencies.py, though it will require us to add some extra CI setup.
|
Thanks very much for the suggestions @agriyakhetarpal When trying the first, unfortunately it returns: |
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, the action repository is hosted under Astral's organisation and not GitHub's.
|
Oops, didn't mean to close! Open again now, but there's a new error? |
|
Yes, this is because the |
|
Triggered a run, and it looks like it's working: https://github.com/pybop-team/PyBOP/actions/runs/21102561524, though it fails because it's somehow trying to install |
|
Great, then the test is working! It should fail for any minimum dependency that is too low. I only updated the numpy/scipy minimum in this PR! So we would expect the action to fail on develop. Thank you! |
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks, @NicolaCourtier!
|
Oh dear, same error after merging... looks like I can't just leave the numpy minimum unspecified (I assumed it would be be contemporary with the scipy version) |
|
Ah, yeah... I think a safe minimum for us would be NumPy 1.25.x series. I can't think of a reason we will use anything below that. |
Description
Add a continuous integration test that employs the lowest-direct dependencies, alongside the existing nightly check on the latest dependencies, to inform the lowest supported versions that we state in the
pyproject.toml.Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #).
Important checks:
Please confirm the following before marking the PR as ready for review:
$ pre-commit runor$ nox -s pre-commit(see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)nox -s testsnox -s doctest